Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 75.77%. Comparing base (c53585e) to head (88a9e3b).

@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
+ Coverage   75.75%   75.77%   +0.02%     
==========================================
  Files          51       51              
  Lines        4190     4203      +13     
==========================================
+ Hits         3174     3185      +11     
- Misses       1016     1018       +2     

@ThomasBreuer any concerns, comments, questions about this

I have no objections.

If I understand the idea right then we can (should?) use the new feature in Oscar as follows.
For an Oscar group G, we can call Oscar.GAPWrap.ConjugacyClasses(G) instead of Oscar.GAPWrap.ConjugacyClasses(GapObj(G)).
We cannot call Oscar.GAPWrap.Size(G) instead of Oscar.GAPWrap.Size(GapObj(G)) because Size does not get wrapped with the requirement that its argument is a GapObj.
Thus the Oscar code will look a bit funny if we omit the unnecessary GapObj calls for arguments of Oscar.GapWrap.Something functions.

But we could change the wrapper for Size to require a GAPObj. In fact there is a ton of wrappers we could do that. I was simply lazy when setting up most of them.

Looking at the list in Oscar/src/GAP/wrappers.jl, my understanding is that most (all?) of the ::Any arguments can be safely changed to ::GAP.Obj, assuming that there are no methods for the GAP functions in question that take other Julia objects.

This change would not help us to get an automatic indirection from Oscar objects to underlying GAP objects.

Writing ::GapObj instead would probably fit to the way the functions are intended for Oscar, although the GAP functions in question really admit any input --the IsSomething filters are of that kind. We could go this way.

We may be looking at different subsets of the wrappers? Because my view is that the majority would be safe to use GapObj, as no Int, Bool or FFE makes sense as argument for e.g. Size, IsFreeGroup, Image (for the first argument at least, the second is different), and a ton more.

That said, as I already suggested in the feature request leading to this: we could also add code to deal with GAP.Obj and GAP.GapInt. Indeed, we just have to add a few more elseif to the code in this PR to catch those.

I added something similar as your suggestions as comments in the different cases.

Perhaps a test could be added?

There already is a doctest coercing a julia string to a GAP string. For tests for GAP.Obj and GAP.GapInt I am afraid that I don't know enough about the types to find suitable GAP functions to test this properly.